Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Auto Suggest] DQL Updates #7593

Merged
merged 19 commits into from
Aug 27, 2024

Conversation

paulstn
Copy link
Contributor

@paulstn paulstn commented Jul 31, 2024

Description

Update to DQL Autocomplete: #7391

Every file removal under grammar/.antlr can be ignored.

  • use official value suggestion methods instead of direct api call
  • removed language specified configuration
  • added memoization for value suggestion to reduce number of calls made
  • removed core start from query suggestion function
  • added tests for value suggestion
  • added more test coverage for other general cases
  • [RFC] Monaco Code Editor provider registration #7594 made changes based on this RFC
  • remove grammar/.antlr auto generated files
  • updated types in code completion and related files
  • fixed many group value suggestion bugs and edge cases with more robust parser visitor
  • fix group value NOT suggestion bugs
  • added basic keyword syntax highlighting

Issues Resolved

Screenshot

Testing the changes

Changelog

  • fix: Update DQL Autocomplete in code and functionality

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

Attention: Patch coverage is 78.68852% with 13 lines in your changes missing coverage. Please review.

Project coverage is 64.19%. Comparing base (1976ecf) to head (0c2888a).
Report is 146 commits behind head on main.

Files with missing lines Patch % Lines
...c/plugins/data/public/antlr/dql/code_completion.ts 81.35% 0 Missing and 11 partials ⚠️
...s/data/public/autocomplete/autocomplete_service.ts 0.00% 1 Missing ⚠️
...ashboards_react/public/code_editor/code_editor.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7593      +/-   ##
==========================================
+ Coverage   64.17%   64.19%   +0.01%     
==========================================
  Files        3659     3658       -1     
  Lines       80796    80814      +18     
  Branches    12866    12874       +8     
==========================================
+ Hits        51848    51875      +27     
+ Misses      25769    25757      -12     
- Partials     3179     3182       +3     
Flag Coverage Δ
Linux_1 30.28% <0.00%> (-0.02%) ⬇️
Linux_2 55.89% <ø> (ø)
Linux_3 40.70% <80.00%> (+0.03%) ⬆️
Linux_4 31.44% <0.00%> (-0.02%) ⬇️
Windows_1 30.30% <0.00%> (-0.02%) ⬇️
Windows_2 55.85% <ø> (ø)
Windows_3 40.71% <80.00%> (+0.04%) ⬆️
Windows_4 31.44% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

2 similar comments
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@github-actions github-actions bot added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Jul 31, 2024
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
…groupings

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
…cases

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
@paulstn paulstn marked this pull request as ready for review August 26, 2024 21:45
joshuali925
joshuali925 previously approved these changes Aug 26, 2024
{ text: '_index', type: 3, insertText: '_index: ', end: -1, start: -1 },
{ text: '_score', type: 3, insertText: '_score: ', end: -1, start: -1 },
{ text: '_source', type: 3, insertText: '_source: ', end: -1, start: -1 },
{ text: '_type', type: 3, insertText: '_type: ', end: -1, start: -1 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should constants used for test be in a *.test.ts file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that since the file didn't contain any tests, it shouldn't have that extension. I couldn't find any precedence for constants being used exclusively for tests, do you know if there is another way of handling these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah @joshuali925 similar comment https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7593/files#r1732297731

you can use mock or just include this in the test.

exporting public fieldNameSuggestions could get confusing for other plugins since now any plugin can access it and this will get compiled down with the release distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense, i'll move everything here into those tests

@kavilla
Copy link
Member

kavilla commented Aug 27, 2024

seeing some failures in the ci.

also qq, i know @mengweieric was working on a feature branch for his updates. does these updates have those and account for changes related to that work. since it has clean ups?

{ text: '_score', type: 3, insertText: '_score: ' },
{ text: '_source', type: 3, insertText: '_source: ' },
{ text: '_type', type: 3, insertText: '_type: ' },
export const fieldNameSuggestions: Array<{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't store test values in constants file. constants will get bundled up and will increase the release distribution size. test files do not get compiled with the distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, could you point me to where its specified which files don't get compiled in the distribution? would it just be .test.ts files, or are there other files that wouldn't be included?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some hard coded ignores in the repo, like

'--ignore',
'**/*.test.ts,**/*.test.tsx',

@@ -63,7 +61,7 @@ export class AutocompleteService {
const provider = this.querySuggestionProviders.get(language);

if (provider) {
return provider({ core: this.core, ...args });
return provider(args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i'm reverting a change that i made previously with dql autocomplete. this was before i knew about a clear way to get to the http core util. but now since we're using services, its redundant and so i put it back the way it was.

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
@paulstn
Copy link
Contributor Author

paulstn commented Aug 27, 2024

also qq, i know @mengweieric was working on a feature branch for his updates. does these updates have those and account for changes related to that work. since it has clean ups?

The updates here and the interface changes Eric made in the feature branch don't conflict with each other, since I've included the necessary changes to DQL Autocomplete that handle those changes within that feature branch.

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
@paulstn
Copy link
Contributor Author

paulstn commented Aug 27, 2024

seeing some failures in the ci.

The cypress failures seem to be coming and going for different tests with each trivial, unrelated commit. I'm not sure what I should be doing here other than retry until all the stars align.

joshuali925
joshuali925 previously approved these changes Aug 27, 2024
body: JSON.stringify({ query: currentValue, field: fieldName, boolFilter: [] }),
});
};

const findValueSuggestions = async (
index: IndexPattern,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the indexpattern service or just object. because actually can just take the query object which now has the dataset object which has all the information you will need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual object, I believe its set by the query_editor.tsx function which comes from the dataset object.

kavilla
kavilla previously approved these changes Aug 27, 2024
@github-actions github-actions bot removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Aug 27, 2024
@joshuali925 joshuali925 merged commit 741c0d6 into opensearch-project:main Aug 27, 2024
53 of 54 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 27, 2024
Update to DQL Autocomplete: #7391

Every file removal under `grammar/.antlr` can be ignored.

- [x] use official value suggestion methods instead of direct api call
- [x] removed language specified configuration
- [ ]  ~~added memoization for value suggestion to reduce number of calls made~~
- [x] removed core start from query suggestion function
- [x] added tests for value suggestion
- [x] added more test coverage for other general cases
- [ ] ~~[[RFC] Monaco Code Editor provider registration #7594](#7594) made changes based on this RFC~~
- [x] remove grammar/.antlr auto generated files
- [x] updated types in code completion and related files
- [x] fixed many group value suggestion bugs and edge cases with more robust parser visitor
- [x] fix group value NOT suggestion bugs
- [ ] ~~added basic keyword syntax highlighting~~

---------

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 741c0d6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ashwin-pc pushed a commit that referenced this pull request Aug 28, 2024
Update to DQL Autocomplete: #7391

Every file removal under `grammar/.antlr` can be ignored.

- [x] use official value suggestion methods instead of direct api call
- [x] removed language specified configuration
- [ ]  ~~added memoization for value suggestion to reduce number of calls made~~
- [x] removed core start from query suggestion function
- [x] added tests for value suggestion
- [x] added more test coverage for other general cases
- [ ] ~~[[RFC] Monaco Code Editor provider registration #7594](#7594) made changes based on this RFC~~
- [x] remove grammar/.antlr auto generated files
- [x] updated types in code completion and related files
- [x] fixed many group value suggestion bugs and edge cases with more robust parser visitor
- [x] fix group value NOT suggestion bugs
- [ ] ~~added basic keyword syntax highlighting~~

---------



(cherry picked from commit 741c0d6)

Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants